Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Removed temp files once they are no longer needed from hubble and xmm #2465

Merged
merged 3 commits into from
Jul 20, 2022
Merged

Removed temp files once they are no longer needed from hubble and xmm #2465

merged 3 commits into from
Jul 20, 2022

Conversation

javier-ballester
Copy link
Contributor

Dear Astroquery,

These changes have been made to fix issue #2107. Please let me know if any further changes are needed to fix this issue.

Many thanks,

Javier

@codecov
Copy link

codecov bot commented Jul 13, 2022 •
edited
Loading

Codecov Report

Merging #2465 (ae9de56) into main (f2d0bd2) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #2465   +/-   ##
=======================================
  Coverage   62.92%   62.92%           
=======================================
  Files         133      133           
  Lines       17302    17302           
=======================================
  Hits        10888    10888           
  Misses       6414     6414

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@javier-ballester javier-ballester marked this pull request as ready for review July 13, 2022 14:16
keflavich
keflavich previously approved these changes Jul 13, 2022
View reviewed changes
Copy link
Contributor

@keflavich keflavich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, though probably it is safer (in the future?) to use tempfile/tempdir machinery for this?

@eerovaher
Copy link
Member

With the proposed solution files will only be cleaned up if the tests don't fail. It would be better to use the pytest tmp_path fixture.

@bsipocz bsipocz added this to the v0.4.7 milestone Jul 13, 2022
bsipocz
bsipocz requested changes Jul 13, 2022
View reviewed changes
Copy link
Member

@bsipocz bsipocz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please use the tempdir solution the other modules also use, it's more resilient for various testing scenarios.

@keflavich keflavich dismissed their stale review July 14, 2022 06:20

tempdir approach is more robust and is needed

@javier-ballester
Copy link
Contributor Author

Dear @bsipocz, I have added the changes that you have requested. Let me know if there are any more changes that are needed.

Many thanks,

Javier

eerovaher
eerovaher reviewed Jul 19, 2022
View reviewed changes
astroquery/esa/hubble/tests/test_esa_hubble.py Outdated
parameters = {'observation_id': "J6FL25S4Q",
'cal_level': "RAW",
'filename': "J6FL25S4Q.vot.test",
'filename': tempdir + "/" + "J6FL25S4Q.vot.test",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this work on Windows as intended or will / be interpreted as a part of the file name?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Usually windows will automatically correct to a backslash if a forward slash is detected but sometimes this is not the case.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll directly add a commit that changes this to os.path.join, and will go ahead and merge it then rather than go into more rounds.
Thanks @javier-ballester!

bsipocz
bsipocz reviewed Jul 20, 2022
View reviewed changes
Copy link
Member

@bsipocz bsipocz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are still a few files showing up once the tests are run, I'll see whether I can patch it really quick and get this merged.

@bsipocz bsipocz force-pushed the ESA_removing_temp_files branch from 6fa74ba to ae9de56 Compare July 20, 2022 18:50
@bsipocz
Copy link
Member

bsipocz commented Jul 20, 2022 •
edited
Loading

OK, so let's go ahead and merge this. The remote tests are still generating files, but for those, the download data functionality may need to be patched instead (or making the esa code such as a donwload/save dir can be specified rather than rely on the logic that everything gets dumped into the working directory).

@bsipocz bsipocz merged commit 74fb75d into astropy:main Jul 20, 2022
@bsipocz
Copy link
Member

bsipocz commented Jul 20, 2022

Thanks @javier-ballester!

@bsipocz bsipocz mentioned this pull request Jul 20, 2022
Closed
@javier-ballester javier-ballester deleted the ESA_removing_temp_files branch July 21, 2022 07:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eerovaher eerovaher eerovaher left review comments

@bsipocz bsipocz bsipocz approved these changes

@keflavich keflavich keflavich left review comments

Assignees
No one assigned
Labels
esa.esa_hubble esa.xmm_newton no-changelog-entry-needed testing
Projects
None yet
Milestone
v0.4.7
Development

Successfully merging this pull request may close these issues.
4 participants
@javier-ballester @eerovaher @bsipocz @keflavich